Skip to content

N°9383 - CheckToWrite inside a transition: Incorrect error message display#837

Open
accognet wants to merge 1 commit intosupport/3.2from
feature/9383-CheckToWrite_message_in_transition
Open

N°9383 - CheckToWrite inside a transition: Incorrect error message display#837
accognet wants to merge 1 commit intosupport/3.2from
feature/9383-CheckToWrite_message_in_transition

Conversation

@accognet
Copy link
Contributor

internal

@CombodoApplicationsAccount CombodoApplicationsAccount added the internal Work made by Combodo label Mar 10, 2026
@accognet accognet requested a review from bdalsass March 10, 2026 17:29
@accognet accognet changed the base branch from develop to support/3.2 March 10, 2026 17:30
@greptile-apps
Copy link

greptile-apps bot commented Mar 10, 2026

Greptile Summary

This PR bundles three distinct fixes under the N°9383 ticket: a targeted error-handling improvement in the stimulus/transition flow, a security hardening in the synchronization import page, and an information-disclosure removal from the Universal Search page. Test dependencies are also bumped as a maintenance task.

  • pages/UI.php: A new catch (CoreCannotSaveObjectException $e) block is inserted before the existing catch (CoreException $e) block. Because CoreCannotSaveObjectException extends CoreException, without this specific handler the exception was previously caught generically and error messages were retrieved via getMessage(). The new handler calls $e->getIssues() to retrieve the structured issues array, which is the purpose of the specialized exception. The rollback logic (reload object + re-apply posted form fields) is identical to the CoreException path. One minor style note: implode(' ', $e->getIssues()) joins issues with a plain space, while getMessage() would produce ', '-separated output; using a clearer delimiter would improve readability.
  • pages/UniversalSearch.php: Removes the OQL filter being written into an HTML comment (<!-- $sFilter -->), closing an information-disclosure path that could expose data-model internals to anyone viewing page source.
  • synchro/synchro_import.php: Replaces the 'raw_data' sanitization filter for data_source_id with utils::ENUM_SANITIZATION_FILTER_INTEGER, ensuring the parameter is properly sanitized before use.
  • tests/php-unit-tests/composer.lock: Routine dev-dependency updates (PHPUnit 9.5→9.6, nikic/php-parser 4→5, doctrine/instantiator 1.5→2.0, various sebastian/* packages). doctrine/instantiator 2.0.0 now requires PHP ≥ 8.1 — this only affects test execution environments, not the runtime application.

Confidence Score: 4/5

  • This PR is safe to merge; all changes are targeted fixes or housekeeping with no breaking runtime impact.
  • The core logic change (new catch block) is correct — CoreCannotSaveObjectException is a subclass of CoreException so the ordering is right, and the rollback is consistent with the existing handler. The only non-critical concern is the implode(' ', ...) separator choice. The security fixes in UniversalSearch.php and synchro_import.php are straightforward and clearly beneficial. Composer lock bumps are dev-only.
  • Pay attention to pages/UI.php — specifically that the $sIssues separator used in the new catch block produces less readable output than the existing getMessage() approach.

Important Files Changed

Filename Overview
pages/UI.php Adds a dedicated catch (CoreCannotSaveObjectException $e) block before the generic CoreException handler so that getIssues() is used to extract the structured error list; rollback logic mirrors the existing handler. Minor: separator used in implode differs from the string already built by getMessage().
pages/UniversalSearch.php Removes the OQL filter being leaked as an HTML comment (<!-- $sFilter -->), which is a clean security improvement preventing data model / query exposure in the page source.
synchro/synchro_import.php Replaces the unsafe 'raw_data' sanitization filter with the proper utils::ENUM_SANITIZATION_FILTER_INTEGER constant for data_source_id, preventing non-integer values from being accepted.
tests/php-unit-tests/composer.lock Routine test-dependency bumps including PHPUnit 9.5→9.6, nikic/php-parser 4.x→5.x, doctrine/instantiator 1.5→2.0 (now requires PHP ≥8.1), and several sebastian/* packages. These are dev-only dependencies with no runtime impact.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User submits Stimulus form] --> B{Transaction valid\n& Stimulus allowed?}
    B -- No --> Z[Show error message]
    B -- Yes --> C[UpdateObjectFromPostedForm]
    C --> D{CheckToWrite passes?}
    D -- No --> E[sIssues = implode issues array\nShow form again + error modal]
    D -- Yes --> F[ApplyStimulus]
    F -- Success --> G[bApplyStimulus = true\nShow success message]
    F -- CoreCannotSaveObjectException --> H[NEW: Rollback object\nsIssues = implode getIssues\nShow form again + error modal]
    F -- CoreException --> I[Rollback object\nsIssues = getMessage\nShow form again + error modal]
    F -- bApplyStimulus = false --> J[Show FailedToApplyStimuli error]
Loading

Last reviewed commit: 1f86062

@jbostoen
Copy link
Contributor

If working on this - perhaps also a good time to take another look at #666 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Work made by Combodo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants